-
Notifications
You must be signed in to change notification settings - Fork 241
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add organizer selection #6251
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6251 +/- ##
============================================
- Coverage 23.88% 23.76% -0.13%
Complexity 454 454
============================================
Files 246 246
Lines 11656 11715 +59
Branches 2189 2207 +18
============================================
Hits 2784 2784
- Misses 8557 8614 +57
- Partials 315 317 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
b4c1bca
to
8e83a72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works
When switching calendars the selection is slightly lagging. E.g. when you go to a shared calendar, select the calendar owner as organizer and then go to an owned calendar the other person is still shown as organizer.
Interesting, looks like the change calendar event is not triggering a refresh. I will look in to this... Also will need to re-test all this after the changes design asked me to make today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works but the UX could be improved. Currently, the select is always shown even if there is no organizer. There is no placeholder because it is always pre-filled with the current user (principal) so a user might not get what the field is doing.
It's great to see you getting into frontend development. I left some feedback regarding common patterns and conventions in the vue world. Nothing big really.
8e83a72
to
14cf46c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SebastianKrupinski I didn’t mean for the organizer to have a menu with all the people, but for all the menus of the people to have another option called "Organizer". :) Same for Talk with the "Promote to moderator" action in the menu of every participant.
Calendar menu | Talk menu |
---|---|
Morning @jancborchardt, At the moment the only users that can be selected as an Organizer are the current logged in user (Sharee) and the calendar owner (Sharer). You cannot set just any person as an organizer, due to the internal mechanics of CalDav. That is why I went with a menu on the organizer object, that only lists the permitted organizers. |
@SebastianKrupinski got it. Then we simply need a menu entry on the organizer with 1 action saying:
And since only 2 people can be organizer, it will always be the other person in the action. Makes sense? |
@jancborchardt no problem I can do that. |
How is this? Personally I think the wording should say 'Make (Persons Name) the Organizer' |
Promote sounds like user 2 will become organizer too but it will take the organizer role from user 1, right? |
Correct. Only one person can be the organizer. So User 1 loses the organizer role. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, then to fix the issue @ChristophWurst mentions, @SebastianKrupinski your wording suggestion sounds good:
Make (Persons Name) the organizer
Note "organizer" lowercase because we use "Sentence case" as per design guidelines: https://docs.nextcloud.com/server/latest/developer_manual/design/foundations.html#wording
Done. |
66a921e
to
718694e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, seems good to me! :)
It feels strange to have the Make X organizer in the actions menu of Y. Shouldn't Make X organizer in the action menu of attendee X? Moreover, I think there is a small bug.
-> share owner becomes organizer, but I'm fully removed and am not listed as attendee. I would expect demoted organizers become attendees |
That can be done, but would be very cumbersome in a manager assistant role, you would need to search for the manager then add the manager as an attendee every time then convert the attendee to an organizer.
The default selected organizer could have been disabled on shared calendars, if we used drop downs or an action menu in a different location, but with the design asking for the selection to be on the organizer, that's not really possible.
The most plausible scenarios for someone else making an appointment for the calendar owner are a assistant role, therefor
Which one of those scenarios is most used? I have no clue. But maybe we could add two options to the menu, one for each scenario. "Make (person) the organizer" and "Make (person) the organizer and attend" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks better now and works.
I think the two button idea from our latest call is missing.
It is going to work on that now. |
b15d280
to
e8048dd
Compare
@st3iny please give this one final test, then let's get this in |
Signed-off-by: SebastianKrupinski <[email protected]> Signed-off-by: Richard Steinmetz <[email protected]>
e8048dd
to
cbfbda2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm gonna fix the remaining things myself.
Works otherwise.
Resolves #6238